[SDK Ergonomics] NEXUS-519: Support Query-backed Nexus Operations - #11274
[SDK Ergonomics] NEXUS-519: Support Query-backed Nexus Operations#11274mavemuri wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
c6bd436 to
d0784fb
Compare
| go.opentelemetry.io/otel/sdk/metric v1.43.0 | ||
| go.opentelemetry.io/otel/trace v1.44.0 | ||
| go.temporal.io/api v1.63.4 | ||
| go.temporal.io/api v1.62.15-0.20260622232249-060670b1b866 |
There was a problem hiding this comment.
placeholder until api change is merged
52ae6e5 to
2657210
Compare
|
Claude finished @mavemuri's task in 7m 1s —— View job Review complete
Overall the change looks sound: the |
| // Note: QueryWorkflow should not alter mutable state, so it is safe to ignore error and not clear ms. | ||
| workflowLease.GetReleaseFn()(nil) | ||
| }() | ||
| defer func() { |
There was a problem hiding this comment.
common/links.Validate has no *commonpb.Link_Workflow_ case — it falls into default: and returns InvalidArgument: "unsupported link variant" (common/links/validator.go:60-61).
That makes the link this PR returns a dead end for the main thing links are for: taking a link you got back and attaching it to a follow-up request. A client that does
qResp, _ := c.WorkflowService().QueryWorkflow(ctx, ...)
c.WorkflowService().StartWorkflowExecution(ctx, &workflowservice.StartWorkflowExecutionRequest{
..., Links: []*commonpb.Link{qResp.GetLink()},
})gets InvalidArgument: unsupported link variant. Same for SignalWorkflowExecution, SignalWithStart, and UpdateWorkflowExecution (service/frontend/workflow_handler.go:693, 2241, 2286, 2350, 2445) plus the CHASM validators (chasm/lib/workflow/validator.go:220, chasm/lib/activity/link_validator.go:34).
Strictly this gap predates the PR — updateworkflow already emits Link_Workflow for rejected updates — but this change makes the variant reachable from every query response, so it's worth closing here. Adding the case alongside the Link_Activity_ one is a few lines:
case *commonpb.Link_Workflow_:
if t.Workflow.GetNamespace() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty namespace field")
}
if t.Workflow.GetWorkflowId() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty workflow ID field")
}
if t.Workflow.GetRunId() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty run ID field")
}
| go.opentelemetry.io/otel/sdk/metric v1.43.0 | ||
| go.opentelemetry.io/otel/trace v1.44.0 | ||
| go.temporal.io/api v1.63.4 | ||
| go.temporal.io/api v1.62.15-0.20260622232249-060670b1b866 |
There was a problem hiding this comment.
Beyond the replace placeholder you already flagged below: this line also downgrades the require from v1.63.4 (which main just moved to in #11264) to the older v1.62.15-0.20260622232249-060670b1b866 pseudo-version. The replace masks that today, so it's easy to miss — but if the replace is dropped without also restoring the require, the module silently regresses to a pre-v1.63.4 API.
When temporalio/api#842 lands, the cleanup is: delete the replace, set this back to the released tag (>= v1.63.4), and restore the two go.temporal.io/api lines that were removed from go.sum.
2657210 to
ad84e98
Compare
mavemuri
added
request-claude-review
mavemuri
added
request-claude-review
For Nexus Operations. Query is both read-only and sync- so 1. no callbacks 2. the link is only from caller to the handler- and is specifically a workflow link as there isnt a history event in handler for query <!-- Describe what has changed in this PR --> **What changed?** Added link field to QueryWorkflowResponse <!-- Tell your future self why have you made these changes --> **Why?** Required for QueryWorkflow-backed Nexus Operations <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** None <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** temporalio/temporal#11274 (Note: no server breakage)
For Nexus Operations. Query is both read-only and sync- so 1. no callbacks 2. the link is only from caller to the handler- and is specifically a workflow link as there isnt a history event in handler for query <!-- Describe what has changed in this PR --> **What changed?** Added link field to QueryWorkflowResponse <!-- Tell your future self why have you made these changes --> **Why?** Required for QueryWorkflow-backed Nexus Operations <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** None <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** temporalio/temporal#11274 (Note: no server breakage)
b4aff7c to
2e97eb4
Compare
mavemuri
added
request-claude-review
2e97eb4 to
66ea079
Compare
66ea079 to
7c59a1e
Compare
7c59a1e to
203efe9
Compare
mavemuri
added
request-claude-review
long-nt-tran
left a comment
long-nt-tran
left a comment
There was a problem hiding this comment.
I think we definitely need a couple Nexus-based query test here as well. Off the top of my head, I think we should at least test:
- Ongoing workflow, well-formed query (happy path)
- Ongoing workflow, malformed query (correct error returned)
- Completed workflow, well-formed query (and under different
QueryRejectConditionstates) - Completed workflow, malformed query (and under different
QueryRejectConditionstates)
When I did linking for signals I put that under tests/nexus_workflow_test.go, but feel free to make a new tests/nexus_query_test.go instead of piling on.
For example, here's how we'd set up such a Nexus-based test -- I'm using a Nexus signal test as a reference:
- Map the handler (on the callee side) to a query (example with signal)
- Map the Nexus op (on the callee side) to a query (example with signal)
- Have a caller workflow invoke the Nexus operation (example with signal)
- In my test I validated that the signal shows up in history, but for query I think you can validate that the query successfully returns, or some validation that indicates the expected success/failure outcome(s)
| s.Equal("pauseabc", queryResultStr) | ||
| } | ||
|
|
||
| func (s *QueryWorkflowSuite) TestQueryWorkflowResult_ContainsWorkflowLink() { |
There was a problem hiding this comment.
maybe this can be in tests/links_test.go since we have other link-related tests there
Agree, had added more cases in the sdk-go PR temporalio/sdk-go#2508 |
3 participants
What changed?
Adds server support for WorkflowQuery-backed Nexus Operations
Why?
Part of effort to expose all Temporal primitives as Nexus Operations
How did you test it?
TODO: